[linter-miner] feat(linters): add bytesbufferstring linter — flag string(buf.Bytes()) in favour of buf.String()#45301
Conversation
Reports string(buf.Bytes()) calls where buf is *bytes.Buffer, suggesting buf.String() instead. The .String() method avoids an unnecessary allocation by reading the underlying byte slice directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds a custom Go analyzer that replaces string(buf.Bytes()) with buf.String().
Changes:
- Implements and registers the
bytesbufferstringanalyzer. - Adds diagnostics, suppression handling, and automatic fixes.
- Adds analysistest fixtures and golden output.
Show a summary per file
| File | Description |
|---|---|
cmd/linters/main.go |
Registers the analyzer. |
pkg/linters/bytesbufferstring/bytesbufferstring.go |
Implements detection and fixes. |
pkg/linters/bytesbufferstring/bytesbufferstring_test.go |
Runs analyzer tests. |
pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go |
Provides test cases. |
pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go.golden |
Defines expected fixes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
| if receiverType == nil { | ||
| return | ||
| } | ||
| if !isBytesBufferPtr(receiverType) { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: bytesbufferstring linter The implementation is well-structured and follows existing linter conventions closely. The type-checking via isBytesBufferPtr correctly handles both bytes.Buffer value and *bytes.Buffer pointer receivers, the nolint suppression works, and the golden-file test covers the key cases. One non-blocking issue found: the analyzer registration order in multichecker.Main(...) (see inline comment) — bytesbufferstring should appear before bytescomparestring to match the alphabetical order used elsewhere in that list and already used in the import block.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 27.7 AIC · ⌖ 6.98 AIC · ⊞ 4.8K
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 30.6 AIC · ⌖ 4.49 AIC · ⊞ 6.6K
Comment /matt to run again
| }) | ||
| }) | ||
|
|
||
| return nil, nil |
There was a problem hiding this comment.
[/tdd] Missing test for a named type that embeds bytes.Buffer — isBytesBufferPtr only checks obj.Name() == "Buffer", so a type like type MyBuf bytes.Buffer (or an interface wrapping it) would silently pass through unchecked.
💡 Suggested test fixture addition
Add to testdata/src/bytesbufferstring/bytesbufferstring.go:
type MyBuffer bytes.Buffer
func GoodStringConversionAliasedType() string {
var buf MyBuffer
// string(buf.Bytes()) should NOT fire — MyBuffer is not bytes.Buffer
_ = buf
return ""
}The intent is to confirm the linter does not fire on types that merely share a name or embed the buffer, preventing potential future false positives if isBytesBufferPtr is extended.
@copilot please address this.
| TextEdits: []analysis.TextEdit{{ | ||
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| NewText: []byte(receiverText + ".String()"), |
There was a problem hiding this comment.
[/codebase-design] Function name isBytesBufferPtr is misleading — the function also handles value receivers (bytes.Buffer, not just *bytes.Buffer), as the comment confirms. Consider renaming to isBytesBuffer or isBytesBufferType to match the actual semantics.
@copilot please address this.
| func BadStringOfBytesCall() string { | ||
| var buf bytes.Buffer | ||
| buf.WriteString("hello") | ||
| return string(buf.Bytes()) // want `string\(buf\.Bytes\(\)\) can be simplified to buf\.String\(\)` |
There was a problem hiding this comment.
[/tdd] Missing test case for a chained or indirect receiver: getBuffer().Bytes() where getBuffer() returns *bytes.Buffer. This is a realistic code pattern and confirming it fires (or does not fire intentionally) clarifies the intended scope.
💡 Suggested addition
func getBuffer() *bytes.Buffer { return &bytes.Buffer{} }
func BadStringOfBytesCallIndirect() string {
return string(getBuffer().Bytes()) // want `string\(getBuffer\(\)\.Bytes\(\)\) can be simplified to getBuffer\(\)\.String\(\)`
}If the current astutil.NodeText call returns the full receiver text including the call expression, this will already work — adding a fixture case makes that explicit.
@copilot please address this.
There was a problem hiding this comment.
The linter logic is correct: type resolution via pass.TypesInfo, pointer-unwrapping in isBytesBufferPtr, and the nolint suppression path all work as expected. No false-positive or false-negative correctness bugs found.
Non-blocking observations
Misleading helper name
isBytesBufferPtr handles both *bytes.Buffer and bytes.Buffer (value). The name implies pointer-only, which will confuse the next person reading if !isBytesBufferPtr(receiverType) for a value-typed receiver. Rename to isBytesBufferType.
Test fixture coverage
Only simple local-variable receivers (var buf bytes.Buffer, buf := &bytes.Buffer{}) are exercised. Struct-field and chained receivers (w.buf.Bytes()) are not tested. The silent if receiverText == "" guard means failures in astutil.NodeText for more complex nodes would silently drop diagnostics rather than surface as test failures.
🔎 Code quality review by PR Code Quality Reviewer · 32.6 AIC · ⌖ 4.57 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/linters/bytesbufferstring/bytesbufferstring.go:137
Misleading function name: isBytesBufferPtr also accepts bytes.Buffer (non-pointer), which the comment confirms, but the name implies pointer-only.
<details>
<summary>💡 Suggested fix</summary>
Rename to isBytesBufferType (or isBytesBuffer) to match what it actually does:
// isBytesBufferType reports whether t is *bytes.Buffer or bytes.Buffer.
func isBytesBufferType(t types.Type) bool {A maintainer reading if !isBytesBufferPtr(receiverType) would reasonably assume va…
pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go:34
Test coverage gap: no test case for struct-field or method-chained receivers, which are common real-world patterns.
<details>
<summary>💡 Suggested additions</summary>
Add to the test fixture to verify the linter fires and auto-fixes on more complex receiver expressions:
type Wrapper struct {
buf bytes.Buffer
}
func BadStructFieldReceiver(w *Wrapper) string {
return string(w.buf.Bytes()) // want `string\(w\.buf\.Bytes\(\)\) can be simplified to w\.buf\.String\(\)`
}W…
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Test Classification
Detailed Analysis
The test file has proper Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (206 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…*bytes.Buffer The rewrite string(buf.Bytes()) → buf.String() is not semantics-preserving when buf is a nil *bytes.Buffer: the former panics while the latter returns "<nil>". Restrict the linter to only flag bytes.Buffer value receivers. - Rename isBytesBufferPtr → isBytesBufferValue; no longer unwraps pointers - Update Analyzer.Doc, package doc comment, and inline comments - Remove the want annotation from BadStringOfBytesCallPtr test case - Add NilPointerBuf fixture showing nil *bytes.Buffer is not flagged - Update golden file to match new fixture Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the nil-pointer semantics issue in commit |
|
Run: https://github.com/github/gh-aw/actions/runs/29289965782
|
…rect-receiver fixtures - cmd/linters/main.go: register bytesbufferstring.Analyzer before bytescomparestring.Analyzer (alphabetical order) - testdata: add wrappedBuffer (embedded bytes.Buffer) fixture — confirms linter does not fire on non-bytes.Buffer named types - testdata: add getBufferPtr() indirect pointer-receiver fixture — confirms linter does not fire when receiver is *bytes.Buffer - golden: update to match new no-op fixtures Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit
CI needs a human re-trigger (close/reopen or |
test